Skip to content

Conversation

sebrandon1
Copy link
Member

Related to: openshift/enhancements#1773

This is my first attempt at a FeatureGate so I'm expecting this to need a bunch of work.

Additions to API Specification and Validation:

  • Added HTTP01ChallengeProxy configuration to the APIServerSpec struct, allowing users to enable and configure the HTTP01 challenge proxy for API endpoint certificates. This includes options for DefaultDeployment and CustomDeployment modes. (config/v1/types_apiserver.go, [1] [2]

Feature Gate Registration:

  • Registered the new HTTP01ChallengeProxy feature gate in features.go, enabling it in the TechPreviewNoUpgrade scope and providing metadata such as a contact person and enhancement proposal link. (features/features.go, features/features.goR869-R876)

Test Cases for Validation:

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 1, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Aug 1, 2025

@sebrandon1: This pull request references CNF-13731 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set.

In response to this:

Related to: openshift/enhancements#1773

This is my first attempt at a FeatureGate so I'm expecting this to need a bunch of work.

Additions to API Specification and Validation:

  • Added HTTP01ChallengeProxy configuration to the APIServerSpec struct, allowing users to enable and configure the HTTP01 challenge proxy for API endpoint certificates. This includes options for DefaultDeployment and CustomDeployment modes. (config/v1/types_apiserver.go, [1] [2]

Feature Gate Registration:

  • Registered the new HTTP01ChallengeProxy feature gate in features.go, enabling it in the TechPreviewNoUpgrade scope and providing metadata such as a contact person and enhancement proposal link. (features/features.go, features/features.goR869-R876)

Test Cases for Validation:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

openshift-ci bot commented Aug 1, 2025

Hello @sebrandon1! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 1, 2025
@openshift-ci openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 4, 2025
@sebrandon1 sebrandon1 force-pushed the feature_gate branch 5 times, most recently from be55466 to cb15245 Compare August 4, 2025 22:19
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2025
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised to see so many protobuf changes here. Something seems off with this many changes being made.

@JoelSpeed is probably more familiar with what might be going on here than I.

Because you are making changes to a CRD, you should be able to run PROTO_OPTIONAL=true make update to make generation updates without touching the protobuf stuff

@JoelSpeed
Copy link
Contributor

Proto changes appear to be unrelated, verify thinks changing them is wrong and is suggesting to change them back

Possibly running with a newer or incompatible version?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2025
@openshift-ci openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 6, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2025
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 6, 2025
@sebrandon1
Copy link
Member Author

Okay I started over and re-ran it with PROTO_OPTIONAL=true make update like you suggested and it seems much more manageable now. I'll take whatever suggestions I can get 😄

Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the linter and test issues are fixed, LGTM

@JoelSpeed
Copy link
Contributor

API types look good, but it looks like some of the test cases are expecting things that aren't true (optional port), so lets get those fixed up and please squash the various fixups

@sebrandon1 sebrandon1 force-pushed the feature_gate branch 5 times, most recently from d6a8c58 to 654300a Compare August 26, 2025 20:13
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some comments based on my analysis of the CI failures

@everettraven
Copy link
Contributor

False positive on new required field with optional parent.

/override ci/prow/verify-crd-schema

Copy link
Contributor

openshift-ci bot commented Aug 27, 2025

@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crd-schema

In response to this:

False positive on new required field with optional parent.

/override ci/prow/verify-crd-schema

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@sebrandon1 sebrandon1 force-pushed the feature_gate branch 3 times, most recently from b67caf9 to f30ea16 Compare August 28, 2025 21:12
@JoelSpeed
Copy link
Contributor

Changes here LGTM, there are however some code generation issues by the looks of it

Fix HTTP01ChallengeProxy integration test structure and enable feature gate in TechPreviewNoUpgrade

- Fixed test file structure to include required crdName metadata for integration tests
- Enabled HTTP01ChallengeProxy feature gate in both DevPreviewNoUpgrade and TechPreviewNoUpgrade
- Regenerated feature gate manifests via make update
- Resolved CI failure: missing required field crdName in test spec

Make internalPort optional to resolve API compatibility error

- Changed internalPort from required to optional (*int32 with omitempty)
- This resolves the NoNewRequiredFields API compatibility violation
- Updated test case to reflect optional field behavior
- Users can now omit internalPort for custom deployments
- Regenerated deepcopy functions and OpenAPI schemas

Address comments 1

Update codegen crds

Adjust back to required

Remove pointer

Address comments for linter
@everettraven
Copy link
Contributor

/test minor-images

@everettraven
Copy link
Contributor

/approve
/lgtm

verify-crd-schema failing on a false positive. New required fields are all parented by optional fields.

/override ci/prow/verify-crd-schema

Holding this PR on merge of enhancement proposal. @JoelSpeed Feel free to overrule me here if holding on EP merge is not necessary.
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2025
Copy link
Contributor

openshift-ci bot commented Sep 2, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, sebrandon1

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

openshift-ci bot commented Sep 2, 2025

@everettraven: Overrode contexts on behalf of everettraven: ci/prow/verify-crd-schema

In response to this:

/approve
/lgtm

verify-crd-schema failing on a false positive. New required fields are all parented by optional fields.

/override ci/prow/verify-crd-schema

Holding this PR on merge of enhancement proposal. @JoelSpeed Feel free to overrule me here if holding on EP merge is not necessary.
/hold

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2025
Copy link
Contributor

openshift-ci bot commented Sep 2, 2025

@sebrandon1: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-serial-techpreview-2of2 aef739d link true /test e2e-aws-serial-techpreview-2of2
ci/prow/e2e-aws-ovn-hypershift aef739d link true /test e2e-aws-ovn-hypershift

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants